Fix #10103 - CheckConvexity removes colinear vertices but stops when surface is determined non-convex, causing vertex size mismatch fatal error #10114
Conversation
```
[ RUN ] EnergyPlusFixture.SurfaceGeometry_CheckConvexity_ColinearStability
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3331: Failure
Expected equality of these values:
6
floorSurface.Sides
Which is: 8
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3350: Failure
Expected equality of these values:
6
ceilingSurface.Sides
Which is: 9
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3354: Failure
Expected equality of these values:
floorSurface.Sides
Which is: 8
ceilingSurface.Sides
Which is: 9
```
…surface is determined non-convex, causing vertex size mismatch fatal error
| if (state.dataGlobal->DisplayExtraWarnings && surfaceTmp.ExtSolar && | ||
| (state.dataHeatBal->SolarDistribution != DataHeatBalance::Shadowing::Minimal) && | ||
| // Warn only once | ||
| surfaceTmp.IsConvex) { | ||
| ShowWarningError(state, | ||
| format("CheckConvexity: Zone=\"{}\", Surface=\"{}\" is non-convex.", |
There was a problem hiding this comment.
When warning, do it only once (when surfaceTmp.IsConvex, before that's set to false)
| surfaceTmp.IsConvex = false; | ||
| break; | ||
| // #10103 - We do not want to break early, because we do want to consistently remove colinear vertices | ||
| // to avoid potential vertex size mismatch fatal errors | ||
| // break; |
There was a problem hiding this comment.
And do NOT break, so we ensure we do drop all colinear vertices and do not end up with a vertex size mistmatch.
| // Test for #10103 - Regardless of the order of the vertices, we should drop colinear vertices consistently to avoid a fatal error due to vertex | ||
| // side mismatch | ||
| // | ||
| // y | ||
| // ▲ 7 8 9 | ||
| // │ ┌─────x────┐ | ||
| // │ │ │ | ||
| // │ │ │ | ||
| // │ │ │ | ||
| // │5 │ │ | ||
| // ├──────────┘ │ | ||
| // │ 6 │ | ||
| // │ │ | ||
| // │ │ | ||
| // │ │10 | ||
| // └────x─────x─────x────┴─────► | ||
| // 4 3 2 1 x |
There was a problem hiding this comment.
Unit test case. We expect 1, 2, 3, and 8 to be dropped, resulting in 6 vertices in the end.
There was a problem hiding this comment.
Before the fix, the floor is checked in the order show above. so it drops 2 and 3, then finds a non convex situation when checking (5, 6, 7) and bails. We end up with 8 vertices.
For the ceiling, the vertices are reversed. So it drops 8 (shown above, 3 in actual order), finds the non convex situation at (7, 6, 5) (shown above, (4, 5, 6 in actual order)) and bails. We end up with 9 vertices.
(I explained the same on #10103 when I reported the defect)
| EXPECT_FALSE(ceilingSurface.IsConvex); | ||
| } | ||
|
|
||
| EXPECT_EQ(floorSurface.Sides, ceilingSurface.Sides); |
There was a problem hiding this comment.
Before fix:
[ RUN ] EnergyPlusFixture.SurfaceGeometry_CheckConvexity_ColinearStability
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3331: Failure
Expected equality of these values:
6
floorSurface.Sides
Which is: 8
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3350: Failure
Expected equality of these values:
6
ceilingSurface.Sides
Which is: 9
/home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/SurfaceGeometry.unit.cc:3354: Failure
Expected equality of these values:
floorSurface.Sides
Which is: 8
ceilingSurface.Sides
Which is: 9
|
I don't think we need to hold this up any longer, this is great. Thanks @jmarrec ! |
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.